Tighten canonical-repo match in queryTargetsDependingOnModules#338
Merged
tinder-maxwellelliott merged 1 commit intoMay 19, 2026
Merged
Conversation
rdark
force-pushed
the
fix/query-modules-canonical-match
branch
2 times, most recently
from
e031d63 to
1f737d4
Compare
Collaborator
|
@rdark There is a conflict now. Thanks again for this |
rdark
force-pushed
the
fix/query-modules-canonical-match
branch
2 times, most recently
from
0d4922c to
9c3b424
Compare
Contributor
Author
|
@tinder-maxwellelliott fixed up the conflicts and simplified this one a bit - thanks! |
Collaborator
|
@rdark I think I addressed some of this from another issue, sorry for another conflict |
Contributor
Author
|
No worries! Will take another look tomorrow |
Layered on top of Tinder#335 fixes (Tinder#353 parse-asymmetry guard, Tinder#354 base-canonical-repo predicate), this commit hardens several edge cases in the surrounding code. * Restructure detectChangedModules to return Set<Module> instead of module-key strings, threading module.name straight to the canonical-match predicate. * Fix `computeSimpleImpactedTargets(emptyMap(), allTargets)` returning allTargets.keys, silently unioning every target back into the rdeps result. Thread `from` so the union uses the real hash diff. * Reject empty module.name in ModuleGraphParser.extractModules. The unnamed-root MODULE.bazel case (where `bazel mod graph --output=json` emits name="") would otherwise reach canonicalRepoIsBaseForModule and over-match every canonical. * Downgrade per-module "no external repository found" from WARN to INFO and emit a single aggregate at the end. * Make the rdeps-failure fallback shape-aware for bzlmod-only workspaces. The unconditional !startsWith("@@") filter left only //external:* bridges, which the downstream excludeExternalTargets default (Tinder#334) then strips to empty. Replace with a shape-aware fallback: emit the buildable subset (//... minus //external:) when non-empty, otherwise every hashed label. Extracted as isBuildableWorkspaceTarget. * New tests in CalculateImpactedTargetsInteractorModuleQueryTest cover substring no-over-match, sentinel for the execute path, both branches of the shape-aware fallback, canonical dedup on add+remove version bumps, and the excludeExternalTargets interaction with the catch-fallback. New parser test covers empty-name rejection.
rdark
force-pushed
the
fix/query-modules-canonical-match
branch
from
9c3b424 to
00e83da
Compare
Contributor
Author
|
@tinder-maxwellelliott fixed up the remaining parts of this PR that still have some value post #334 |
tinder-maxwellelliott
approved these changes
May 19, 2026
Collaborator
tinder-maxwellelliott
left a comment
tinder-maxwellelliott
left a comment
There was a problem hiding this comment.
Thank you
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Partial fix for #335.
Layered on top #353 (parse-asymmetry guard), #354 (base-canonical-repo predicate). Hardens several edge cases in the surrounding code:
Set<Module>restructuring.detectChangedModulesnow returns typedModules instead of module-key strings, wiringmodule.nameup to the canonical-match predicate.Real hash diff in the rdeps union. Fix
computeSimpleImpactedTargets(emptyMap(), allTargets)returningallTargets.keysand silently unioning every target back into the rdeps result. Wire upfromso the union uses the real hash diff.reject empty
module.nameat the parser. The unnamed-rootMODULE.bazelcase (wherebazel mod graph --output=jsonemitsname="") would otherwise reachcanonicalRepoIsBaseForModuleand over-match every canonical.Shape-aware rdeps-failure fallback. The unconditional
!startsWith("@@")filter left only//external:*bridges on bzlmod-only workspaces, whichexcludeExternalTargetsdefault (from Fix #326: filter //external:* labels from impacted-targets output on bzlmod #334) then strips to empty. Instead emit the buildable subset (//...minus//external:*) when non-empty, otherwise every hashed label. Extracted asisBuildableWorkspaceTarget.Log-noise downgrade. Per-module "no external repository found" goes from
WARNtoINFO, plus a single aggregateINFOat the end.Tests in
CalculateImpactedTargetsInteractorModuleQueryTest: substring no-over-match, sentinel forexecute, both branches of the shape-aware fallback, canonical dedup on add+remove version bumps, and theexcludeExternalTargetsinteraction with the catch-fallback. NewModuleGraphParserTestcase for empty-name rejection.Changes vs the previous PR draft: